Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use nextstrain/ingest (git subtree) #162

Closed
wants to merge 30 commits into from

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Jul 28, 2023

Description of proposed changes

Begin usage of centralized ingest scripts and add details on how to pull new updates from the central repo.

Related issue(s)

Superseded by #164.

Testing

  • Ingest workflow runs locally without errors
  • Test with upload/send_slack_notifications/trigger_rebuild?

tsibley and others added 30 commits July 6, 2023 13:06
Add minimum CI workflow that currently only runs a shellcheck job like
the shellcheck workflow in the ncov-ingest repo¹.

We may choose to add more tests in the future, but shellcheck is enough
for now to catch errors in bash scripts. This will be helpful as I
centralize the scripts that have diverged in the various ingest
workflows.

¹ https://github.com/nextstrain/ncov-ingest/blob/6fd5a9b1d87e59fab35173dbedf376632154943b/.github/workflows/shellcheck.yml
Copied from https://github.com/nextstrain/ncov-ingest/blob/8442ba80bc2ab16c345db1ad53233542fca343fe/bin/notify-slack

Subsequent copies of this script that are functionally identical:
- https://github.com/nextstrain/forecasts-ncov/blob/b7229bad08b047d480a2b40c4bafb1e33b7fe84b/ingest/bin/notify-slack
- https://github.com/nextstrain/rsv/blob/ba171f4a43110382c38b6154be3febd50408d7bf/ingest/bin/notify-slack

There was a one line difference in the rsv script that will be omitted
in this repo:
```
@@ -1,4 +1,5 @@
 #!/bin/bash
+# Originally copied from nextstrain/ncov-ingest repo
 set -euo pipefail

 : "${SLACK_TOKEN:?The SLACK_TOKEN environment variable is required.}"
```
Changes copied from 8757890

Subsequent copies of this script that contain identical changes:
- https://github.com/nextstrain/dengue/blob/247b2fd897361f2548627de1d97d45fae4115c5c/ingest/bin/notify-slack
- https://github.com/nextstrain/zika/blob/4ac8d526f9f14be10b7e8858ad469a40b72a505e/ingest/bin/notify-slack

These changes use optional args so they will not require changes to
calls of the previous version of the script in ncov-ingest,
forecasts-ncov, and rsv.
Copied from https://github.com/nextstrain/monkeypox/blob/5c461dc7e90cd70c1f16b193f82fd1666d4c95e2/ingest/bin/notify-on-job-start

I decided to copy the version in monkeypox because it is easier to
update to be a generalized script than the ncov-ingest version which has
unique changes to support the run-nextclade-full scripts.¹

¹ nextstrain/ncov-ingest@88be153...e898400
Generalize the script by using new args to customize the Slack message
and point to the appropriate GitHub Action URL.

These changes were made based on diffs with subsequent copies of the
script that edited the Slack message:
- https://github.com/nextstrain/rsv/blob/ba171f4a43110382c38b6154be3febd50408d7bf/ingest/bin/notify-on-job-start
- https://github.com/nextstrain/forecasts-ncov/blob/70bf78f459a3706dd817ae5f711af3b74887d7b1/bin/notify-on-job-start
- https://github.com/nextstrain/dengue/blob/247b2fd897361f2548627de1d97d45fae4115c5c/ingest/bin/notify-on-job-start

This will require all calls of the script to be updated since job_name
and repo_name are required args.
The default value is "ingest" which is the expected ingest directory
for standard pathogen repos.

Adding the optional arg to support customizations for historical repos
such as ncov-ingest that do not follow the new standardized repo structure.
Copied from https://github.com/nextstrain/ncov-ingest/blob/6fd5a9b1d87e59fab35173dbedf376632154943b/bin/notify-on-job-fail

The following commits will update and generalize the script to
incorporate changes made subsequent copies.
Follow patterns set in notify-on-job-start where the environment
variables `AWS_BATCH_JOB_ID` and `GITHUB_RUN_ID` are used directly
instead of reassigning them to local variables and use double square
brackets.
Generalize the script by using new args to customize the Slack message
and point to the appropriate GitHub Action URL.

The repo_name arg was added based on diffs with subsequent copies of the
script that edited the Slack message:
- https://github.com/nextstrain/monkeypox/blob/5c461dc7e90cd70c1f16b193f82fd1666d4c95e2/ingest/bin/notify-on-job-fail
- https://github.com/nextstrain/forecasts-ncov/blob/70bf78f459a3706dd817ae5f711af3b74887d7b1/ingest/bin/notify-on-job-fail
- https://github.com/nextstrain/rsv/blob/ba171f4a43110382c38b6154be3febd50408d7bf/ingest/bin/notify-on-job-fail
- https://github.com/nextstrain/dengue/blob/247b2fd897361f2548627de1d97d45fae4115c5c/ingest/bin/notify-on-job-fail
- https://github.com/nextstrain/zika/blob/4ac8d526f9f14be10b7e8858ad469a40b72a505e/ingest/bin/notify-on-job-fail

Although the job_name arg is not necessary based on diffs with
subsequent copies of the script, it's nice to be able to customize the
Slack message for different ingest jobs. It's also a plus to mirror the
args for notify-on-job-start.

This will require all calls of the script to be update since job_name
and repo_name are required args.
I realized in my testing of the script that broadcasting does not
work for file uploads. Confirmed that this is not an argument for the
files.upload API.¹

Including the argument did not cause an error, but better to remove it
so it doesn't confuse us in the future.

¹ https://api.slack.com/methods/files.upload
Using GitHub repos by org/name pair to make scripts more generalizable.
This will also work really well with the `GITHUB_REPOSITORY` variable
available for GitHub Action workflows.

Co-authored-by: Victor Lin <13424970+victorlin@users.noreply.github.com>
Add simple Slack notifications
…3d13181'

git-subtree-dir: ingest/vendored
git-subtree-mainline: 5c461dc
git-subtree-split: 9082700
The previous merge commit and its attached commits were created by the
following command:

    git subtree add --prefix ingest/vendored https://github.com/nextstrain/ingest HEAD

Add a section in the README on how to use this directory in the future.
Remove the copies in this repo and update references.
Remove the copies in this repo and update references.

Add new positional arguments required by the centralized scripts.
@victorlin victorlin self-assigned this Jul 28, 2023
@victorlin victorlin marked this pull request as ready for review July 31, 2023 18:20
@victorlin victorlin requested a review from a team July 31, 2023 18:20
@victorlin victorlin changed the title Use nextstrain/ingest Use nextstrain/ingest (git subtree) Jul 31, 2023
@victorlin victorlin closed this Jul 31, 2023
@victorlin victorlin deleted the victorlin/centralized-ingest-git-subtree branch July 31, 2023 19:10
joverlee521 added a commit to nextstrain/ingest that referenced this pull request Jul 31, 2023
Revisiting our previous decisions in #3
after @victorlin did more testing with the monkeypox repo with
`git subtree` in nextstrain/mpox#162 and
`git subrepo` in nextstrain/mpox#164.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants